-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wrap a single test in test-amp-form.js that expect a console error with allowConsoleError
#14592
Wrap a single test in test-amp-form.js that expect a console error with allowConsoleError
#14592
Conversation
@choumx knows these tests better than I do. |
b85d29f
to
42f7c2d
Compare
As discussed offline, it's possible your test is doing something asynchronously, that's causing the console error to appear after the anonymous function passed to Examples: This will warn...
... as will this...
... but this will work...
... and so will this...
|
@@ -55,13 +55,16 @@ describes.repeated('', { | |||
let sandbox; | |||
const timer = Services.timerFor(window); | |||
|
|||
function getAmpForm(form, canonical = 'https://example.com/amps.html') { | |||
function getUnresolvedAmpForm(form, canonical = 'https://example.com/amps.html') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: getAmpFormSync
.
}; | ||
ampForm.handleSubmitEvent_(event); | ||
const findTemplateStub = ampForm.templates_.findAndRenderTemplate; | ||
allowConsoleError(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the same problem I mentioned in chat. The returned promise won't be chained (unless this has been since fixed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. Not yet fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're referring to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place a breakpoint on line 577. I believe it won't get triggered.
This is because sinon chains returned promises, but allowConsoleError()
currently swallows it. What we need is for allowConsoleError(f)
to detect promises returned in by f
and chain them. Then, we'll do:
return allowConsoleError(() => {
...
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #15228. With this, you can return allowConsoleError
. Improperly written tests will fail presubmit checks.
42f7c2d
to
ac80e57
Compare
Thanks for the Pull Request. Since this PR has been stale for a while, I am closing it. Please feel free to reopen as needed. |
Followup to #14432
This only fixes one test which was previously assigned to me, because each and every test in this file is weird in its own way and I don't want to go down this rabbit hole